Add unshare to podman by weirdwiz · Pull Request #2969 · containers/podman

weirdwiz · 2019-04-18T14:54:30Z

This command lets the user run a command in a new user namespace like unshare -u.
It uses the implementation of unshare in buildah. ( fixes #1388 )

openshift-ci-robot · 2019-04-18T14:54:50Z

Hi @weirdwiz. Thanks for your PR.

I'm waiting for a containers or openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vrothberg · 2019-04-18T14:56:00Z

/ok-to-test
/approve

rh-atomic-bot · 2019-04-18T15:00:36Z

Can one of the admins verify this patch?
I understand the following commands:

bot, add author to whitelist
bot, test pull request
bot, test pull request once

vrothberg · 2019-04-18T15:01:20Z

Hi @weirdwiz, thanks for the contribution!

I did not check the code yet but we also need to add a man page and bash completions for the new command.

rhatdan · 2019-04-18T15:02:13Z

Need update commands.md

rhatdan · 2019-04-18T15:04:15Z

Would be nice to have some tests too if possible

rhatdan · 2019-04-18T15:03:38Z

cmd/podman/unshare.go

PODMAN does not have an ISOLATION type, I think this code can be dropped?
@giuseppe Do you agree?

yes, correct

weirdwiz · 2019-04-18T15:09:06Z

I see I'll add the man pages, bash completions and update the commands.md

Sure @rhatdan I'll make some tests.

edsantiago

A few usability notes.

Also: what is the expected/desired behavior when run as root? I get:

ERRO[0000] error reading allowed ID mappings: error reading subuid mappings for user "root" and subgid mappings for group "root": No subuid ranges found for user "root" in /etc/subuid

edsantiago · 2019-04-18T17:04:56Z

cmd/podman/unshare.go

Should be: "unshare [flags] [COMMAND [ARG...]]" to better indicate usage.

edsantiago · 2019-04-18T17:06:29Z

cmd/podman/unshare.go

Perhaps more helpful as "no command specified, and $SHELL undefined"

edsantiago · 2019-04-18T17:07:22Z

cmd/podman/unshare.go

Why the leading newline?

baude · 2019-04-18T17:55:04Z

cmd/podman/unshare.go

why exit with a 1? And the return following?

It exits with a 1 because ExecRunnable() exits with the exit status of the command, so if the control returns back to the function, it is definitely an error. The return nil is unnecessary I have now removed it.

baude · 2019-04-18T17:55:27Z

cmd/podman/unshare.go

is there no error handling here?

This function does not return anything and automatically exits depending on the exit code of the command run.

baude · 2019-04-18T17:56:18Z

cmd/podman/unshare.go

why not return an error

baude · 2019-04-18T17:56:30Z

cmd/podman/unshare.go

return an error?

baude · 2019-04-18T17:56:47Z

cmd/podman/unshare.go

does this need to be added to the remote client?

Probably not - only makes sense on a local machine, it spawns a fresh terminal in the userns.

Definitely not.

ok, then the command should not go in main. it needs to be added to the commands.go instead so it only shows up for the local client.

giuseppe · 2019-04-19T07:49:17Z

we need to re-use the unshare implementation of Podman, where we join the existing namespace if it already exists. This should happen automatically if you just implement unshare to setup the shell or execute the specified command without caring of setting up the userns.

giuseppe · 2019-04-19T07:49:58Z

cmd/podman/unshare.go

yes, correct

giuseppe · 2019-04-19T07:50:20Z

cmd/podman/unshare.go

if you drop this line, would podman setup the namespace automatically?

Yes, It sets up the namespace automatically, If I remove this line.

mheon · 2019-04-30T17:43:13Z

The unshare-specific test seems to be failing, and I'd like a head nod from @giuseppe - but otherwise this seems good to go.

openshift-ci-robot · 2019-05-02T15:48:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, vrothberg, weirdwiz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mheon · 2019-05-02T17:50:50Z

Might push this to 1.4 so @giuseppe can give an ack

rh-atomic-bot · 2019-05-02T18:46:30Z

☔ The latest upstream changes (presumably #3039) made this pull request unmergeable. Please resolve the merge conflicts.

weirdwiz · 2019-05-03T01:56:54Z

Rebased

rhatdan · 2019-05-03T09:09:49Z

I have no idea why this is failing, and the logs seem to be useless.

You might need to add
podmanTest.CgroupManager = "cgroupfs"
podmanTest.StorageOptions = ROOTLESS_STORAGE_OPTIONS

But I am not sure.
@giuseppe @baude PTAL
These tests pass for me when run locally.

giuseppe · 2019-05-09T13:52:54Z

we should disable the tests when running as remote with SkipIfRemote().

@weirdwiz can you please take care of it?

weirdwiz · 2019-05-09T23:49:15Z

@giuseppe I have added SkipIfRemote() now

giuseppe · 2019-05-16T07:49:11Z

could you please rebase?

This command lets the user run a command in a new user namespace like `unshare -u`. It uses the implementation of unshare in buildah. ( fixes containers#1388 ) Signed-off-by: Divyansh Kamboj <kambojdivyansh2000@gmail.com>

giuseppe · 2019-05-16T09:48:47Z

tests are happy. LGTM

vrothberg · 2019-05-16T10:33:44Z

Nice work, @weirdwiz!
/lgtm

openshift-ci-robot requested review from umohnani8 and vrothberg April 18, 2019 14:54

openshift-ci-robot added the size/M label Apr 18, 2019

openshift-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 18, 2019

openshift-ci-robot added ok-to-test approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 18, 2019

rhatdan reviewed Apr 18, 2019

View reviewed changes

edsantiago suggested changes Apr 18, 2019

View reviewed changes

baude reviewed Apr 18, 2019

View reviewed changes

cmd/podman/unshare.go Outdated

Copy link

Member

baude Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not return an error

baude reviewed Apr 18, 2019

View reviewed changes

cmd/podman/unshare.go Outdated

Copy link

Member

baude Apr 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return an error?

baude reviewed Apr 18, 2019

View reviewed changes

giuseppe reviewed Apr 19, 2019

View reviewed changes

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L and removed size/M needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 21, 2019

weirdwiz force-pushed the master branch from 4883285 to f712178 Compare April 21, 2019 10:29

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L labels Apr 21, 2019

weirdwiz force-pushed the master branch 2 times, most recently from cdc0035 to 006a2f7 Compare April 26, 2019 14:26

weirdwiz force-pushed the master branch from 006a2f7 to 2952603 Compare May 1, 2019 14:22

edsantiago approved these changes May 2, 2019

View reviewed changes

weirdwiz force-pushed the master branch from 2952603 to 225b502 Compare May 3, 2019 00:06

weirdwiz force-pushed the master branch from 225b502 to 5b4bd16 Compare May 3, 2019 14:15

mheon added Release Notes 1.4.0 and removed Release Notes 1.3.0 labels May 3, 2019

weirdwiz force-pushed the master branch 2 times, most recently from 5565730 to 4833b00 Compare May 9, 2019 16:33

Add unshare to podman

2a961a7

This command lets the user run a command in a new user namespace like `unshare -u`. It uses the implementation of unshare in buildah. ( fixes containers#1388 ) Signed-off-by: Divyansh Kamboj <kambojdivyansh2000@gmail.com>

weirdwiz force-pushed the master branch from 4833b00 to 2a961a7 Compare May 16, 2019 08:12

openshift-ci-robot assigned vrothberg May 16, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 16, 2019

openshift-merge-robot merged commit 2bb1487 into containers:master May 16, 2019

mheon added Release Notes 1.3.1 and removed Release Notes 1.4.0 labels May 16, 2019

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 26, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023

Conversation

weirdwiz commented Apr 18, 2019

Uh oh!

openshift-ci-robot commented Apr 18, 2019

Uh oh!

vrothberg commented Apr 18, 2019

Uh oh!

rh-atomic-bot commented Apr 18, 2019

Uh oh!

vrothberg commented Apr 18, 2019

Uh oh!

rhatdan commented Apr 18, 2019

Uh oh!

rhatdan commented Apr 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weirdwiz commented Apr 18, 2019

Uh oh!

edsantiago left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giuseppe commented Apr 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mheon commented Apr 30, 2019

Uh oh!

openshift-ci-robot commented May 2, 2019

Uh oh!

mheon commented May 2, 2019

Uh oh!

rh-atomic-bot commented May 2, 2019

Uh oh!

weirdwiz commented May 3, 2019

Uh oh!

rhatdan commented May 3, 2019

Uh oh!

giuseppe commented May 9, 2019

Uh oh!

weirdwiz commented May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

giuseppe commented May 16, 2019

Uh oh!

giuseppe commented May 16, 2019

Uh oh!

vrothberg commented May 16, 2019

weirdwiz commented May 9, 2019 •

edited

Loading